Repository navigation
fix inconsistent syntax highlighting for field of type inspect._ParameterKind #4751 - #4794
asukaminato0721 wants to merge 5 commits into
Conversation
|
This pull request has been imported. If you are a Meta employee, you can view this in D118576862. (Because this pull request was imported automatically, there will not be any future comments.) |
This comment has been minimized.
This comment has been minimized.
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
I don't love this special-case. until now, all attributes could be colored based on only their type. is this the only exception? Is it true that accessed_on_class + being an enum are the only qualifiers?
b14a160 to
754c9d4
Compare
This comment has been minimized.
This comment has been minimized.
754c9d4 to
75e597a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
75e597a to
4959be3
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| use crate::common::lsp_args; | ||
| use crate::common::pytorch_root_or_skip; | ||
|
|
||
| const MODULE: &str = "torch/nn/modules/module.py"; |
There was a problem hiding this comment.
what is the result of this benchmark? does this file have enough enum-literal attribute accesses?
There was a problem hiding this comment.
if it's neutral, do we need to commit this benchmark? or should we just run it?
| &ast, | ||
| &|range| self.get_type_trace(handle, range), | ||
| &|range, base_range, name| { | ||
| let ty = self.get_type_trace(handle, range)?; |
There was a problem hiding this comment.
can we move this into a named function? I think it's too big for a closure
| let kind = attribute_symbol_kind_from_type(&ty) | ||
| .to_lsp_semantic_token_type_with_modifiers() | ||
| .0; | ||
| let members = match &ty { |
There was a problem hiding this comment.
is there a reusable API that does this?
| `NoTelemetry` excludes the telemetry backend's per-solve work. Internal timing | ||
| and telemetry event construction still execute. | ||
| For a base/head comparison, copy this benchmark and its registration to the | ||
| base revision, then use the same pinned source, compiler, and arguments. |
There was a problem hiding this comment.
If we keep the benchmark, can we drop the NoTelemetry and base/head copy notes? They describe how this PR was measured rather than how to use the benchmark.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
kinto0
left a comment
kinto0
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
Thanks for the fix! It looks good. We are merging it now, and we are handling a few small items in follow-up diffs that land with it:
|
|
This pull request has been merged in 172de12. |
Summary
Fixes #4751
fixes incorrect semantic highlighting for narrowed enum instance attributes.
Previously, holder.kind could be classified as an enumMember after narrowing.
Now instance attributes such as holder.kind remain property, actual enum members accessed through the enum class, such as E.A, remain enumMember.
Test Plan
add test